Skip to content

Add spec test functionality #15

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Jul 29, 2020
Merged

Conversation

juliusgeo
Copy link
Contributor

No description provided.

@juliusgeo juliusgeo requested review from prashantmital and ShaneHarvey and removed request for prashantmital July 21, 2020 19:55
@@ -39,6 +39,8 @@ def command_name(self):
def get_SON(self):
cmd = SON([(self.command_name, self.collection)])
cmd.update(self.command_document)
if self.command_document == {}:
return {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a few cases where the filter was {} and the expected command payload in that case was also {}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be possible for a command to be empty. Could you post those cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in the following cases:

SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which spec test is this specifically? Can you post the entire test failure output?

@@ -51,8 +53,13 @@ def __init__(self, collection: Collection, filter, update,
value = kwargs[key]
if key == "bypass_document_validation":
return_document[key] = value
elif key == "hint":
return_document["updates"][0]["hint"] = value if \
isinstance(value, str) else SON(value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not have the same behavior as Pymongo. Check out how the Collection class handles the "hint" option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused about this comment. This matches the output produced by Pymongo.

Copy link
Collaborator

@ShaneHarvey ShaneHarvey Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out _index_document and where that method is used: https://github.com/mongodb/mongo-python-driver/blob/956ce3d4b0edfa9c1d946109db743f82ed0bfc0a/pymongo/helpers.py#L79

We also need to use _index_document for sort.

else:
return_document["updates"][0][key] = value
if return_document["updates"][0].get("hint", True) == {}:
del return_document["updates"][0]["hint"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding and then removing "hint" how about we only add it if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it to only be added when needed results in 1 test failing, not sure how to fix it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you post those failures here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran 106 tests in 11.719s

FAILED (failures=1, skipped=42)
SON([('update', 'test'), ('updates', [{'q': {'_id': 1}, 'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}], 'upsert': False, 'hint': {}, 'multi': False}])])
SON([('update', 'test'), ('ordered', True), ('lsid', {'id': Binary(b'\xca\xa9L\x9d\x84\x86L\xc7\x9f\xfa\x8cE4\xa8Kq', 4)}), ('txnNumber', 1), ('$clusterTime', {'clusterTime': Timestamp(1595449666, 16), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}), ('$db', 'crud-tests'), ('$readPreference', {'mode': 'primary'}), ('updates', [SON([('q', {'_id': 1}), ('u', [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}]), ('multi', False), ('upsert', False)])])])


[{'multi': False,
  'q': {'_id': 1},
  'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
  'upsert': False}] != [{'hint': {},
  'multi': False,
  'q': {'_id': 1},
  'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
  'upsert': False}]

@@ -81,7 +88,12 @@ def __init__(self, collection: Collection, pipeline, session,
super().__init__(collection.name)
self.command_document = {"pipeline": pipeline, "cursor": cursor_options}
for key, value in kwargs.items():
self.command_document[key] = value
if key == "batchSize":
if value == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check for None too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None values are already removed in the converting to snakecase step

@@ -110,18 +122,30 @@ def __init__(self, collection: Collection,
super().__init__(collection.name)
for key, value in kwargs.items():
self.command_document[key] = value
if self.command_document["filter"] == {}:
self.command_document = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? What about all the other find command options? Like:

coll.find({}, limit=10)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the only way to make all the tests pass, so I'm not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for coll.find({}, limit=10) to ensure that the limit field (and other command args) are not removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test.

if "replacement" in self.command_document.keys():
self.command_document["update"] = self.command_document[
"replacement"]
del self.command_document["replacement"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more readable to construct the command correctly from the start rather than constructing an invalid command and then patching it up after.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused about this comment. The command is being constructed in this step. The kwargs value of replacement has to be patched in that way otherwise it will simply not produce the correct output. If we did not put this code there, then it would have to be duplicated multiple times in explainable_collection.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are certain processing steps that must be followed to match pymongo's output, and most of them seem rather arbitrary so the only way for me to verify is to match my code to the test cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm trying to get at is that it's confusing to add the "replacement" field to command_document and then immediately remove it. Instead I think this class should handle all the findAndModify specific options while we constructing the command. Like this:

    command_document = {
        "findAndModify": collection_name,
        "query": filter,
        "new":  return_document,
        "update": replacement,
    }
    collation = validate_collation_or_none(kwargs.pop('collation', None))
    if collation is not None:
        cmd["collation"] = collation
    if projection is not None:
        cmd["fields"] = helpers._fields_list_to_dict(projection,
                                                     "projection")
    if sort is not None:
        cmd["sort"] = helpers._index_document(sort)
    if upsert is not None:
        common.validate_boolean("upsert", upsert)
        cmd["upsert"] = upsert
    if hint is not None:
        if not isinstance(hint, str):
            hint = helpers._index_document(hint)
        cmd['hint'] = hint
    if array_filters is not None:
        cmd["arrayFilters"] = array_filters

This approach exposes numerous bugs in the current approach. Sometimes an argument name needs validation (like a type assertion), sometimes it needs to be transformed (like collation or hint), and sometimes the python argument name is different from the command argument name (like replacement -> update).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the deleting statements and tried to condense all the logic down into the for loops.

def __init__(self, collection):
self.collection = collection
def __init__(self, collection_object):
self.collection_object = collection_object
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "collection" is a better name. "collection_object" is a bit verbose for Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

@juliusgeo juliusgeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these past few commits I have tried to condense down the logic and get rid of some of the delete statements so that it's easier to follow the construction of commands.

else:
return_document["updates"][0][key] = value
if return_document["updates"][0].get("hint", True) == {}:
del return_document["updates"][0]["hint"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran 106 tests in 11.719s

FAILED (failures=1, skipped=42)
SON([('update', 'test'), ('updates', [{'q': {'_id': 1}, 'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}], 'upsert': False, 'hint': {}, 'multi': False}])])
SON([('update', 'test'), ('ordered', True), ('lsid', {'id': Binary(b'\xca\xa9L\x9d\x84\x86L\xc7\x9f\xfa\x8cE4\xa8Kq', 4)}), ('txnNumber', 1), ('$clusterTime', {'clusterTime': Timestamp(1595449666, 16), 'signature': {'hash': b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00', 'keyId': 0}}), ('$db', 'crud-tests'), ('$readPreference', {'mode': 'primary'}), ('updates', [SON([('q', {'_id': 1}), ('u', [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}]), ('multi', False), ('upsert', False)])])])


[{'multi': False,
  'q': {'_id': 1},
  'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
  'upsert': False}] != [{'hint': {},
  'multi': False,
  'q': {'_id': 1},
  'u': [{'$replaceRoot': {'newRoot': '$t'}}, {'$addFields': {'foo': 1}}],
  'upsert': False}]

if "replacement" in self.command_document.keys():
self.command_document["update"] = self.command_document[
"replacement"]
del self.command_document["replacement"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the deleting statements and tried to condense all the logic down into the for loops.

@@ -110,18 +122,30 @@ def __init__(self, collection: Collection,
super().__init__(collection.name)
for key, value in kwargs.items():
self.command_document[key] = value
if self.command_document["filter"] == {}:
self.command_document = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test.

if key == "bypass_document_validation":
return_document[key] = value
elif key == "hint":
if value is not {} and value is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the {} check here. It's invalid for hint to be {}:

>>>> client.t.t.update_one({}, {'$set': {'a':1}}, hint={})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 1024, in update_one
    hint=hint, session=session),
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 870, in _update_retryable
    _update, session)
  File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1497, in _retryable_write
    return self._retry_with_session(retryable, func, s, None)
  File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1383, in _retry_with_session
    return self._retry_internal(retryable, func, session, bulk)
  File "/Users/shane/git/mongo-python-driver/pymongo/mongo_client.py", line 1415, in _retry_internal
    return func(session, sock_info, retryable)
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 866, in _update
    retryable_write=retryable_write)
  File "/Users/shane/git/mongo-python-driver/pymongo/collection.py", line 806, in _update
    hint = helpers._index_document(hint)
  File "/Users/shane/git/mongo-python-driver/pymongo/helpers.py", line 87, in _index_document
    "mean %r?" % list(iteritems(index_list)))
TypeError: passing a dict to sort/create_index/hint is not allowed - use a list of tuples instead. did you mean []?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

class ExplainCollection():
def __init__(self, collection):
self.collection = collection
self.last_cmd_payload = None

def _explain_command(self, command):
command_son = command.get_SON()
if command_son == {}:
self.last_cmd_payload = {}
return {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed. It's impossible for a command to be empty ({}).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -108,6 +121,8 @@ class FindCommand(BaseCommand):
def __init__(self, collection: Collection,
kwargs):
super().__init__(collection.name)
if kwargs["filter"] == {}:
self.command_document = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug (or it's masking a bug). Let's remove this code and address the failing test in a different way. Please post the test failure here.

For reference this is how pymongo generates a find command:
https://github.com/mongodb/mongo-python-driver/blob/3.11.0rc0/pymongo/message.py#L181-L217

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I thought that might be a bug because it doesn't really make sense. Removing that doesn't seem to cause any additional test failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three different tests that fail are below, they are in the format of <the command we constructed>, <the command that was expected>

SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', True), ('filter', {})])
{}
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', False), ('filter', {})])
{}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you post the entire failed test output though? Like this:

FAIL: test_keyword_arg_defaults (test.test_client.ClientUnitTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/shane/git/mongo-python-driver/test/test_client.py", line 128, in test_keyword_arg_defaults
    self.assertEqual(20.1, pool_opts.connect_timeout)
AssertionError: 20.1 != 20.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SON([('find', 'test_find_allowdiskuse'), ('filter', {})])
{}

Error
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
    return f(*args, **kwargs)
  File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
    self.run_scenario(scenario_def, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
    self.run_test_ops(sessions, collection, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
    self.run_operations(sessions, collection, test['operations'])
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
    result = self.run_operation(sessions, collection, op.copy())
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
    self._compare_command_dicts(wrapped_collection.last_cmd_payload,
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
    self.assertEqual(ours[key], theirs[key])
KeyError: 'find
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', True), ('filter', {})])
{}

Error
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
    return f(*args, **kwargs)
  File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
    self.run_scenario(scenario_def, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
    self.run_test_ops(sessions, collection, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
    self.run_operations(sessions, collection, test['operations'])
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
    result = self.run_operation(sessions, collection, op.copy())
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
    self._compare_command_dicts(wrapped_collection.last_cmd_payload,
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
    self.assertEqual(ours[key], theirs[key])
KeyError: 'find'
SON([('find', 'test_find_allowdiskuse'), ('allowDiskUse', False), ('filter', {})])
{}

Error
Traceback (most recent call last):
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/Cellar/[email protected]/3.8.3_2/Frameworks/Python.framework/Versions/3.8/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/Users/julius/pymongoexplain/test/__init__.py", line 442, in wrap
    return f(*args, **kwargs)
  File "/Users/julius/pymongoexplain/test/test_crud_v2.py", line 62, in run_scenario
    self.run_scenario(scenario_def, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 615, in run_scenario
    self.run_test_ops(sessions, collection, test)
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 525, in run_test_ops
    self.run_operations(sessions, collection, test['operations'])
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 431, in run_operations
    result = self.run_operation(sessions, collection, op.copy())
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 374, in run_operation
    self._compare_command_dicts(wrapped_collection.last_cmd_payload,
  File "/Users/julius/pymongoexplain/test/utils_spec_runner.py", line 269, in _compare_command_dicts
    self.assertEqual(ours[key], theirs[key])
KeyError: 'find'

"updates":[{"q": filter, "u": update}]
})
if upsert is not None:
self.command_document["updates"][0]["upsert"] = upsert
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest refactoring this to:

update_doc = {"q": filter, "u": update}
self.command_document["updates"] = [update_doc]
if upsert is not None:
   update_doc["upsert"] = upsert
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this change. Did it get lost?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored it incorrectly, it is done now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think this is complete. We can replace self.command_document["updates"][0] with update_doc now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

self.command_document = {"pipeline": pipeline, "cursor": cursor_options}
kwargs):

super().__init__(collection.name, kwargs.get("collation", None))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be using kwargs.pop("collation", None) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -76,7 +75,7 @@ def count_documents(self, filter: Document, session=None,
**kwargs):

command = AggregateCommand(self.collection, [{'$match': filter},
{'$group': {'n': {'$sum': 1}, '_id': 1}}],
{'$group': {'n': {'$sum': 1}, '_id': 1}}],
session, {}, kwargs,
exclude_keys=filter.keys())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is exclude_keys?

@@ -63,10 +92,11 @@ def command_name(self):
class DistinctCommand(BaseCommand):
def __init__(self, collection: Collection, key, filter, session,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do some of these classes accept a session but some don't? I think they should all be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

super().__init__(collection.name, kwargs.get("collation", None))
self.command_document.update({"key": key, "query": filter})
if kwargs.get("read_concern", None) is not None:
self.command_document["read_concern"] = kwargs["read_concern"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read_concern is not an option for distinct so we can remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Collaborator

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still some lingering correctness issues here. For example, the sort and projection arguments need to be converted like pymongo does: https://github.com/ShaneHarvey/mongo-python-driver/blob/6932d25639e35a53a67a7960be0603720ce48b00/pymongo/collection.py#L2920-L2924

For example:

>>> client.t.t.find_one(projection=['a', 'b.c'])
>>> client.t.t.find_one(projection={'a': 1, 'b.c': 1})

Please audit these arguments and add tests as you fix these issues to ensure ExplainableCollection works correctly in these cases.

super().__init__(collection.name)
self.command_document = {"query": filter}
def __init__(self, collection: Collection, filter, kwargs,
sxclude_keys=None):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove sxclude_keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

i, exclude_keys=exclude_keys) for i in d[key]]
elif isinstance(d[key], dict):
ret[new_key] = convert_to_camelcase(d[key],
exclude_keys=exclude_keys)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I think we can remove the exclude_keys argument now too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -149,29 +146,30 @@ def find_one_and_delete(self, filter: Document, projection: list = None,
kwargs)
return self._explain_command(command)

def find_one_and_replace(self, filter: Document, replacement: Document,
def find_one_and_replace(self, filter: Document, update:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument should still be called "replacement", not "update". It needs to match Pymongo's find_one_and_replace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

for key, value in kwargs.items():
self.command_document[key] = value
if key == "update" and kwargs.get("replacement", None) is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove all the "replacement" references here because we never pass "replacement" through kwargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacement is passed through kwargs for find_one_and_replace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I was confused. Fixed it now.

Copy link

@prashantmital prashantmital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good now but I am concerned that there might still be lingering bugs in fields that are not tested by our SPEC test suite. @ShaneHarvey do you have any idea if/how we might be able to leverage our PyMongo-native test suite to test such fields?

Copy link

@prashantmital prashantmital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please wait for @ShaneHarvey's approval before merging.

"updates":[{"q": filter, "u": update}]
})
if upsert is not None:
self.command_document["updates"][0]["upsert"] = upsert
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't think this is complete. We can replace self.command_document["updates"][0] with update_doc now.

setup.py Outdated
tests_require=["pymongo==3.10.1"],
install_requires=['pymongo==3.10.1'],
tests_require=["pymongo==3.11.0rc0"],
install_requires=['pymongo==3.11.0rc0'],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be pymongo>=3.11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -20,15 +20,20 @@

from bson.son import SON
from pymongo.collection import Collection
from pymongo.helpers import _index_document, _fields_list_to_dict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though we maintain pymongo we can't rely on any private methods (in python names starting with _ mean private). Could you vendor (copy) these two methods instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@juliusgeo juliusgeo requested a review from ShaneHarvey July 28, 2020 12:53
@juliusgeo juliusgeo requested a review from prashantmital July 28, 2020 19:59
@@ -19,16 +19,71 @@
from typing import Union

from bson.son import SON
from bson.py3compat import abc, iteritems, string_type
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

py3compat is intended as an internal only api within bson. Since we only support python 3 can you remove this and replace each one with the native Python 3-only solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@juliusgeo juliusgeo requested a review from ShaneHarvey July 29, 2020 14:41
Copy link

@prashantmital prashantmital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the crud/v1/tests files before merging - AFAICT we are not running these tests so it makes no sense to have these files in the repo.

Also, I have opened #24 as a follow-up for this work (not part of the 1.0 milestone though).

Great job!

@juliusgeo juliusgeo merged commit 0f16387 into mongodb-labs:master Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants